[Chip] Add `clickable` property #11613

vilvaathibanpb · 2018-05-28T18:16:04Z

Have added a new prop for the Chip - 'clickable' which takes a boolean as input and makes the Chip clickable irrespective of the component type

Closes #11502

mbrookes · 2018-05-28T19:38:54Z

packages/material-ui/src/Chip/Chip.js

      ...other
    } = this.props;

    const className = classNames(
      classes.root,
-      { [classes.clickable]: onClick },
+      { [classes.clickable]: onClick || clickable},


clickable }

mbrookes · 2018-05-28T19:39:27Z

packages/material-ui/src/Chip/Chip.js

@@ -239,10 +240,15 @@ Chip.propTypes = {
   * @ignore
   */
  tabIndex: PropTypes.oneOfType([PropTypes.number, PropTypes.string]),
+  /**
+   * Boolean to determine if the chip should be clickable.


Please follow the wording of other bool props. ('If true, ...')

mbrookes · 2018-05-28T19:47:11Z

packages/material-ui/src/Chip/Chip.js

 };

 Chip.defaultProps = {
  component: 'div',
+  clickable: false


Trailing comma, and clickable goes before component.

mbrookes · 2018-05-28T19:48:09Z

run yarn lint, or npm run lin to see lint issues.

mbrookes · 2018-05-28T20:12:35Z

You'll also need to run yarn docs:api / npm run docs:api to update the API docs markdown from the source code comments. Something I nearly always forget! 😅

Oh, and update the typescript definition (.d.ts file) please.

vilvaathibanpb · 2018-05-29T09:41:30Z

@mbrookes Hi, I am done with other comments, but not able to run yarn docs:api / npm run docs:api. I face an error "module not found 'deepmerge'". Even if I install deepmerge, I get further errors. Can you help me in this?

mbrookes · 2018-05-29T11:41:02Z

Are you using yarn or npm?

Did you run yarn or npm install? For npm 6 you can try npm ci to do a clean install.

vilvaathibanpb · 2018-05-29T11:56:42Z

Hi I am using npm 5.6.0. Yea I did npm install right after I cloned the repo. And after making changes I tried both "yarn docs:api" and "npm run docs:api". But same error. PFA screenshot

mbrookes · 2018-05-29T12:05:13Z

No idea, but since you have yarn, try running yarn && yarn docs:api.

Failing that, I'll do it push to your repo.

vilvaathibanpb · 2018-05-29T12:13:03Z

@mbrookes Wow. That worked. Hopefully its time for me to move to yarn from npm ;-). I guess PR is good right now. Please review it

oliviertassinari · 2018-05-29T12:51:09Z

packages/material-ui/src/Chip/Chip.js

@@ -242,6 +248,7 @@ Chip.propTypes = {
 };

 Chip.defaultProps = {
+  clickable: false,


Should the clickable property have a higher priority than the onClick detection logic?
Meaning, what's the output of <Chip clickable={false} onClick={() => ()} />?

@oliviertassinari I feel clickable should have low priority than OnClick. The output of above will still be clickable as per the below code: { [classes.clickable]: onClick || clickable }, if onclick is present, it will be clickable at any cost. Only when onClick is not present, clickable comes into the picture.

@oliviertassinari This is only affecting the styling (and the description should perhaps reflect that), but requiring the prop to be set in order for a Chip with onClick to appear clickable seems counter-intuitive to me.

but requiring the prop to be set in order for a Chip with onClick to appear clickable seems counter-intuitive to me.

@mbrookes I was asking the question in a context where defaultProps.clickable = null.

Ah, I see. So being able to set clickable={somethingFalse} when a Chip has an onClick() handler but other conditions mean that it won't act on the click?

@oliviertassinari I feel defaultProps.clickable = null also would work fine. still false makes better sense, as we have kept props type as boolean. Also, in terms of functionality, nothing changes in either way

@mbrookes Couldn't understand your question clearly. But to explain, if we have 'onClick' hander, 'clickable' doesn't affect and the chip is always clickable. when 'onClick' is not present, then the value of 'clickable' determine if the chip is clickable

@vilvaathibanpb I was just clarifying with @oliviertassinari if I understood correctly the benefit of defaultProps.clickable = null combined with clickable having priority.

Let me rephrase it as a direct question: @oliviertassinari What do you see as the main benefit for null as the default?

What do you see as the main benefit for null as the default?

The advantage is that it's allowing people to control the clickable state, but still, being able to rely on the onClick detection logic if they wish. But I'm fine either way. The current approach is simple too.

mbrookes · 2018-05-29T20:15:03Z

packages/material-ui/src/Chip/Chip.js

+  /**
+   * If `true`, the chip will be clickable.
+   * By default, the chip will not be clickable.
+   */


Assuming we keep the current logic, the wording needs to be something like:

"If true, the chip will appear clickable, and will raise when pressed, even if the onClick property is not defined. This can be used, for example, along with the component property to indicate an anchor Chip is clickable."

And an example in the docs would be good...

@mbrookes Have changed the comments and added an example as well. Please let me know if any other changes are required

oliviertassinari · 2018-05-30T07:30:43Z

docs/src/pages/demos/chips/Chips.js

@@ -30,6 +30,7 @@ function Chips(props) {
  return (
    <div className={classes.root}>
      <Chip label="Basic Chip" className={classes.chip} />
+      <Chip label="Clickable without onClick Chip" clickable className={classes.chip} />


@mbrookes Unless I'm missing something, by demo, the point was to showcase a link chip no?

Yes. For buttons we link to the page section id, so a link to '#chip' would do.

Got it. you also want me to add component 'a' ? or just adding a 'href="#chip"' will do? And what title will be suitable for it? "Link" or "Clickable without onClick chip (Link)".

Whatever use case you're solving for, and as short a name as possible.

@mbrookes I have added link to the Chip and also made the name as Clickable Link Chip. Please let me know if I have to make any more changes

mbrookes · 2018-05-30T13:59:09Z

docs/src/pages/demos/chips/Chips.js

+        label="Clickable Link Chip"
+        className={classes.chip}
+        clickable
+      />


Please could you put this last in the list of Chips?

In terms of prop ordering, lets put the added props at the end for clarity:

<Chip label="Clickable Link Chip" className={classes.chip} component="a" href="#chip" clickable />

@mbrookes Have rearranged the props. Any other suggestions?

Please could you put this last in the list of Chips?

@mbrookes Sorry I just missed that. Have moved it to the last in the list of Chips. Thanks a lot for your patience as I make so many issues.

vilvaathibanpb · 2018-05-31T12:08:28Z

@mbrookes Please review and let me know if any further changes required. Hopefully, everything is done 👍

mbrookes · 2018-05-31T13:17:09Z

@vilvaathibanpb So far so good, but it looks like you still need to fix the underline?

vilvaathibanpb · 2018-05-31T15:17:26Z

@mbrookes underline as in ? U dont need underline for a Link chip?

vilvaathibanpb · 2018-05-31T15:22:40Z

@mbrookes and @oliviertassinari Ideally this text decoration should be none for chip in the root level right? Not just for clickable

vilvaathibanpb · 2018-05-31T18:40:03Z

@mbrookes Can you check now, please?

oliviertassinari · 2018-05-31T18:47:22Z

For some reason, the new chip link demo isn't keyboard accessible.

vilvaathibanpb · 2018-05-31T19:04:30Z

@oliviertassinari Sure? Because I was able to access the chip with keyboard (Tab key). I use Mac - chrome 64

vilvaathibanpb · 2018-05-31T19:16:18Z

@oliviertassinari Sorry my bad. If you notice, even Basic chip is not accessible through keyboard. Only when onclick or onDelete is present, the chips are keyboard accessible

vilvaathibanpb · 2018-05-31T19:37:14Z

@oliviertassinari Have made the chip accessible through the keyboard. Check now, please.

vilvaathibanpb · 2018-05-31T21:15:53Z

This was my first Open source contribution and i learnt so much about detailing and looking for various checks. Thank you so much for your patience @mbrookes and @oliviertassinari

mbrookes · 2018-05-31T21:19:48Z

@vilvaathibanpb Thanks for seeing this through. I appreciate it was a lot of work for a small change, but hopefully that gives some insight into how much care has gone into the rest of the library. (Which is not to say it's perfect by any means, but we try our best! 😅)

zachrickards · 2018-08-06T17:24:10Z

This would be much better if having clickable={false} OnClick={() => { do something}}. I would like that to disable the click. As of right now, if you have an onclick, clickable does absolutely nothing. The feature that would be much better if styling and making the onclick not function not work. As clickable false, seems like the "chip" should be disabled

vilvaathibanpb · 2018-09-29T19:50:01Z

@mbrookes Should we consider @zachrickards request?

oliviertassinari · 2018-09-29T20:47:44Z

@vilvaathibanpb This sounds like a valid request to me!

mbrookes · 2018-09-29T21:28:27Z

@zachrickards Please open an issue for it so it doesn't get lost. Better still, would you like to work on it?

vilvaathibanpb · 2018-09-29T21:54:07Z

@oliviertassinari and @mbrookes I can pick it up

Fixed Chip should be styled clickable when component is

067e968

vilvaathibanpb mentioned this pull request May 28, 2018

[Chip] Chip should be styled clickable when component is a #11502

Closed

2 tasks

mbrookes requested changes May 28, 2018

View reviewed changes

mbrookes changed the title ~~Fixed Chip should be styled clickable when component is~~ [[Chip] Add clickable prop May 28, 2018

mbrookes changed the title ~~[[Chip] Add clickable prop~~ [Chip] Add clickable prop May 28, 2018

mbrookes reviewed May 28, 2018

View reviewed changes

oliviertassinari added new feature New feature or request component: chip This is the name of the generic UI component, not the React module! labels May 28, 2018

Vilva Athipan added 2 commits May 29, 2018 15:12

Fixed Review comments

ce3d7bb

Fixed Review comments

550f728

Added info in Doc

c0c4114

oliviertassinari reviewed May 29, 2018

View reviewed changes

oliviertassinari approved these changes May 29, 2018

View reviewed changes

oliviertassinari changed the title ~~[Chip] Add clickable prop~~ [Chip] Add clickable property May 29, 2018

mbrookes requested changes May 29, 2018

View reviewed changes

oliviertassinari assigned mbrookes May 29, 2018

Vilva Athipan added 2 commits May 30, 2018 10:50

Changed comments and added an example

fd65f3a

Added API doc

77d4424

oliviertassinari reviewed May 30, 2018

View reviewed changes

Added link to clickable chip

f834c8a

mbrookes requested changes May 30, 2018

View reviewed changes

Vilva Athipan added 2 commits May 31, 2018 08:55

Rearranged the props

ac3ed14

Moved the chip to last of list

8a0645d

Removed Underlining of text in Link chip

0a9867f

Made clickable chips accessible through keyboard

7bd5c43

mbrookes approved these changes May 31, 2018

View reviewed changes

mbrookes merged commit 1bdf0c2 into mui:master May 31, 2018

acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018

[Chip] Add clickable property (mui#11613)

a2edfd2

vilvaathibanpb mentioned this pull request Oct 1, 2018

[Chip] Control clickable property #13056

Merged

[Chip] Add clickable property #11613

[Chip] Add clickable property #11613

Conversation

vilvaathibanpb commented May 28, 2018 • edited by oliviertassinari Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes commented May 28, 2018

mbrookes commented May 28, 2018 • edited Loading

vilvaathibanpb commented May 29, 2018

mbrookes commented May 29, 2018

vilvaathibanpb commented May 29, 2018

mbrookes commented May 29, 2018

vilvaathibanpb commented May 29, 2018

oliviertassinari May 29, 2018 • edited Loading

Choose a reason for hiding this comment

vilvaathibanpb May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vilvaathibanpb commented May 31, 2018

mbrookes commented May 31, 2018

vilvaathibanpb commented May 31, 2018

vilvaathibanpb commented May 31, 2018

vilvaathibanpb commented May 31, 2018

oliviertassinari commented May 31, 2018

vilvaathibanpb commented May 31, 2018

vilvaathibanpb commented May 31, 2018

vilvaathibanpb commented May 31, 2018

vilvaathibanpb commented May 31, 2018

mbrookes commented May 31, 2018

zachrickards commented Aug 6, 2018

vilvaathibanpb commented Sep 29, 2018

oliviertassinari commented Sep 29, 2018

mbrookes commented Sep 29, 2018 • edited Loading

vilvaathibanpb commented Sep 29, 2018 • edited Loading

[Chip] Add `clickable` property #11613

[Chip] Add `clickable` property #11613

vilvaathibanpb commented May 28, 2018 •

edited by oliviertassinari

Loading

mbrookes commented May 28, 2018 •

edited

Loading

oliviertassinari May 29, 2018 •

edited

Loading

vilvaathibanpb May 29, 2018 •

edited

Loading

mbrookes May 29, 2018 •

edited

Loading

mbrookes May 29, 2018 •

edited

Loading

mbrookes commented Sep 29, 2018 •

edited

Loading

vilvaathibanpb commented Sep 29, 2018 •

edited

Loading